Skip to content

Fix type of data parameter in get test requests #267

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Feb 22, 2023
Merged

Fix type of data parameter in get test requests #267

merged 4 commits into from
Feb 22, 2023
+16 −2

Conversation

@nils-van-zuijlen
Copy link
Contributor

@nils-van-zuijlen nils-van-zuijlen commented Oct 18, 2022

Lists of two-tuples of strings are allowed in the data attribute for get requests

Example of a false-positive needing this fix:

        factory = ApiRequestFactory()
        request = factory.get(users_list_url, data=[("manager", manager1.username), ("manager", manager2.username)])

@nils-van-zuijlen
Copy link
Contributor Author

nils-van-zuijlen commented Nov 3, 2022

The import paths seem to be wrong in the original code, hence why the tests are broken.

@intgr
Copy link
Contributor

intgr commented Nov 8, 2022

The CI has been fixed

@nils-van-zuijlen nils-van-zuijlen force-pushed the fix/apirequestfactory-data-type branch from 6b76c3e to 72624b6 Compare November 21, 2022 14:03
@intgr intgr force-pushed the fix/apirequestfactory-data-type branch from 72624b6 to 9f4bc38 Compare December 3, 2022 11:05
@intgr
Copy link
Contributor

intgr commented Dec 3, 2022

CI fixed again, sorry! I rebased your branch.

sobolevn
sobolevn approved these changes Dec 3, 2022
View reviewed changes
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

intgr
intgr requested changes Dec 3, 2022
View reviewed changes
Copy link
Contributor

@intgr intgr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR

@intgr
Copy link
Contributor

intgr commented Dec 3, 2022

A similar change would be helpful in django-stubs as well for the RequestFactory class and django.utils.http.urlencode function.

@intgr
Copy link
Contributor

intgr commented Dec 21, 2022

The lint and test are failing because an import is missing for Mapping, Iterable

But typecheck fails due to changes in upstream DRF tests; I'll fix that in a bit.

@intgr intgr mentioned this pull request Dec 21, 2022
Merged
@nils-van-zuijlen nils-van-zuijlen mentioned this pull request Dec 21, 2022
Merged
@nils-van-zuijlen nils-van-zuijlen force-pushed the fix/apirequestfactory-data-type branch 2 times, most recently from 7eb9c06 to 5f497ee Compare December 21, 2022 15:16
@intgr
Copy link
Contributor

intgr commented Dec 21, 2022

From typecheck output

drf_source/tests/test_pagination.py:546: error: Argument 2 to "get" of "APIRequestFactory" has incompatible type "Dict[str, int]"; expected "Union[Mapping[str, Union[str, bytes, Iterable[Union[str, bytes]]]], Iterable[Tuple[str, Union[str, bytes, Iterable[Union[str, bytes]]]]], None]"

Maybe we should accept int as well? It's weird to pass URL params as integer, but on the other hand, this does work and is covered by unit tests.

If there are still remaining typecheck errors after that, we should just suppress them via IGNORED_ERRORS.

@nils-van-zuijlen nils-van-zuijlen force-pushed the fix/apirequestfactory-data-type branch from 5f497ee to f920e73 Compare January 18, 2023 11:06
@nils-van-zuijlen nils-van-zuijlen mentioned this pull request Jan 22, 2023
Closed
@nils-van-zuijlen @intgr
Fix type of data parameter in get test requests
973b707 
Lists of two-tuples of strings are allowed

Co-authored-by: Marti Raudsepp <[email protected]>
@intgr intgr force-pushed the fix/apirequestfactory-data-type branch from f920e73 to 973b707 Compare January 26, 2023 21:27
intgr
intgr approved these changes Jan 26, 2023
View reviewed changes
Copy link
Contributor

@intgr intgr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I've neglected this project a bit while I focused on django-stubs. Rest assured, I haven't forgotten about it. :)

@intgr
Copy link
Contributor

intgr commented Jan 26, 2023

I rebased your branch, there are some false positives in CI typecheck which shoud probably be added to ignores list.

@intgr intgr self-assigned this Jan 27, 2023
intgr
intgr approved these changes Feb 22, 2023
View reviewed changes
Copy link
Contributor

@intgr intgr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed the relevant typecheck ignores and this can now be merged. Thanks!

@intgr intgr merged commit a038db8 into typeddjango:master Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@intgr intgr intgr approved these changes

@sobolevn sobolevn sobolevn approved these changes

Assignees

@intgr intgr

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nils-van-zuijlen @intgr @sobolevn